Merge train: #9859, #9884, #9885, #9886, #9887 - #9898
Merged
Conversation
added 20 commits
September 6, 2026 16:16
(cherry picked from commit f6e80ad)
(cherry picked from commit 3034937)
(cherry picked from commit c60ba60)
(cherry picked from commit fcc9688)
ECMA-262 ClassDefinitionEvaluation installs class elements in source order, so
a second `get x` / `set x` REPLACES the first. `ClassDecl::getters` / `::setters`
are consumed with `iter().find(...)` — first match wins — but every accessor was
appended with `push()`. The shadowed definition therefore stayed live and the
one the program actually defines last was silently dropped.
Every accessor shape is affected, not just getters. Against
`node --experimental-strip-types`, before this change:
instance getter 111 (expected 222)
static + instance getter instance-first (expected instance-last)
static-first (expected static-last)
duplicate setters first:x (expected last:x)
class expression 1 (expected 2)
There is no diagnostic: the program reads a plausible value from the wrong
accessor and keeps running.
Found in Claude-of-Duty, whose `Spring3` pairs an early `set z` (damping) with
a later `get z` (displacement) — legal, if unusual, and it relies on the
read/write asymmetry the spec produces. Perry served the shadowed damping
getter, so `lag.z` and `recPos.z` read 0.46 and 0.42 (their constructors'
damping arguments) instead of displacements. That added +0.88 m to the
first-person viewmodel's Z, moving the rig from 0.3 m in front of the camera
to 0.58 m behind it. All 156 viewmodel nodes then clipped: the overlay pass ran
and issued every draw, and produced no fragments.
`record_class_accessor` overwrites an existing entry instead of appending. The
replacement is keyed on `(name, is_static)`: a static and an instance accessor
of the same name are distinct properties — one on the constructor, one on the
prototype — and collapsing them would trade this bug for another.
Verified: perry-hir 620 passed, perry-codegen 1912 passed. The regression test
covers all four shapes above and fails on each without this change.
(cherry picked from commit f572ce9)
(cherry picked from commit 36d94d7)
(cherry picked from commit b7f0952)
…fires
Adds the fourth member of the escape-analysis family (escape_news /
escape_arrays / escape_objects): `collectors/segview.rs` recognises
`for (let {segment: O} of X.segment(q))` and proves the segment RECORD never
escapes, so the loop can eventually drive a native cursor instead of
materialising one record per grapheme.
That loop is the target: the allocation census ranks it 1/2/3 by count
(172,032 records + 124,928 + 122,880 substrings per 400-character reply, 58 %
of the top-30 allocation count), and the sample puts 60-85 % of active
main-thread CPU inside it, under ink's wrapText.
What is proven, and what deliberately is not. The only proof is that the
record does not escape — every use of `__destruct_N` is one of the
destructuring field reads the loop head itself emits. A use of the segment
STRING is never a rejection: any use no view entry point answers is served by
materialising the substring once into the same local, which is exactly what
the loop costs today. So `O`'s uses are classified and counted, not gated.
That is what separates "the record is gone" (v1) from "the loop allocates
nothing" (v2, which needs the runtime's regexp_test).
The escape proof is a count, and it is taken with perry_hir's
collect_local_refs_stmt — the LocalId collector that handles every
LocalId-bearing variant explicitly and delegates the rest to the walker whose
match the compiler forces to be exhaustive. A new HIR variant embedding a
LocalGet is therefore a compile error in the walker, not a silently missed use
of the record. The `O`-use classifier is hand-written and can miss a shape, so
it is checked against that same sound count and every unclassified occurrence
is booked as "must materialise": an unrecognised use can make a site look less
optimisable than it is, never more.
No lowering. The tier's fact is populated and unread; the runtime's view-mode
entry points do not exist yet.
The counter is the point of the commit. A tier can be correct and never match
(#9824), so PERRY_SEGVIEW_DIAG=1 reports every for-of site examined, the
verdict, the rejection reason and the per-use tally — and it runs at the
HIR-trace point, the last place before codegen, where the statements scanned
are exactly the statements codegen consumes. That makes "does it fire on the
real bundle?" answerable in HIR-lowering time instead of a full LLVM build.
The env var is excluded from the build-level cache for the same reason
--opt-report is: a cached build never lowers HIR, and a report that prints
nothing reads exactly like a tier that never fired.
Unit tests pin the matcher against the HIR shape a real --trace hir dump
produces, including the two negative controls that matter: a record use hidden
inside a closure rejects, and a `{segment, index}` head declines under its own
name rather than firing.
(cherry picked from commit 3bf49ab)
(cherry picked from commit 23e67fa)
(cherry picked from commit afbf1b7)
… view use
The bundle counter reported `code_point_at=0, materialise=1` for `N$6` in
cli_2.1.112.js -- the string-width loop that is 60-85 % of claude-code's active
main-thread CPU -- where the probe had reported 1 and 0.
Cause: perry's JS pipeline folds `O.codePointAt(k)` into the dedicated
`Expr::StringCodePointAt { string, index }` node. The classifier matched only
the generic `Call(PropertyGet(O, "codePointAt"), [k])` shape, which is what a
TypeScript probe produces. Exactly one occurrence moved buckets, which is the
signature of a single unmatched shape and nothing else.
Two things this does not change: the escape proof (the record's non-escape is a
count from `collect_local_refs_stmt`, not from this classifier) and any
verdict. Only the per-use tally moves, and only in the direction of reporting
more of what the runtime view can answer.
Why the wrong number was visible at all: every occurrence the classifier does
not recognise is reconciled against that sound count and booked as
"must materialise", so an unmatched shape under-reports optimisability and can
never over-report it. A classifier that guessed instead of reconciling would
have reported `code_point_at=0, materialise=0` here and looked correct. That
property is the reason the tallies can be believed.
The rule the miss establishes, now recorded in the module docs: a shape that
reproduces on a probe is not proof it reproduces on the bundle. The bundle
counter is 32 seconds -- run it after every change to this classifier.
(cherry picked from commit 7c6f64b)
… (v1)
v1 per `INTERFACE_segments_view.md` §9b: `js_segments_view_open` + `_next` in
the loop, `_segment` once per step for the body. The body is NOT rewritten, so
every use of the segment binding still sees an ordinary string. This removes
the 48-byte segment RECORD per grapheme -- the allocation census's site 1,
172,032 per 400-character claude-code reply -- and the whole eager
`build_segments` array with its two per-call closures. The substring stays;
per-use `_code_point_at` / `_regexp_test` is the next increment.
Emitted shape, for a site the matcher proves:
Let recv = <receiver> // hoisted, evaluated ONCE
Let inp = <input> // hoisted, evaluated ONCE
Let cur = js_segments_view_open(recv, inp) // 0.0 on decline
Let A = cur != 0 ? undefined : GetIterator(recv.segment(inp))
For { init: Let R = cur != 0 ? _next(cur) : js_for_of_next(A),
cond: cur != 0 ? R == 1 : !R.done,
update: R = cur != 0 ? _next(cur) : js_for_of_next(A),
body: [Let O = cur != 0 ? _segment(cur) : R.value.segment,
<original body, untouched>] }
Three properties this shape exists to get right, each with a test.
The receiver and the input are HOISTED. Both appear on the accept path as
`open`'s arguments and on the decline path as `recv.segment(inp)`, so leaving
them in place would evaluate them twice: `getSegmenter().segment(next())` would
call each twice. That is a miscompile, and claude-code's own `rR_.segment(q)`
would never have exposed it because both operands there are side-effect-free.
The `.segment` PROPERTY GET stays inside the decline arm. Hoisting the receiver
does not hoist the member access, so a receiver whose `segment` is an accessor
runs it exactly once, in its original position, on the path that needs it --
the ordering obligation §9f places on `open`'s decline path, honoured from the
compiler side.
The body is left byte-identical. That is what keeps `break` / `continue` /
labels correct and avoids duplicating any `Expr::Closure` the body contains,
which would carry a duplicate `FuncId`.
The ternaries are real branches: `lower_conditional` emits a four-block CFG
with a phi, so the decline arm's `GetIterator` does not run when `open`
accepted. Verified before relying on it -- an eager select-style lowering would
build the `Segments` on every loop and lose the entire per-call saving.
Fresh LocalIds are seeded above every id the module mentions, declarations
included and not only references: a local declared and never read still owns
its id.
DEFAULT OFF, behind `PERRY_SEGVIEW=1`. The runtime's view entry points do not
exist yet, so an on-by-default rewrite would emit calls that fail to link.
(cherry picked from commit 6dc4d24)
…as lowered to
The counter and the rewrite answered two different questions and only one was
being reported. `PERRY_SEGVIEW_DIAG=1` reports the CLASSIFICATION -- what each
use of the segment binding could be answered by -- and it runs before the
rewrite, because after it the shape is gone. So there was no way to confirm
what was actually EMITTED, which was the counter's original purpose.
Reordering the passes would trade one blind spot for the other. Instead the
rewrite reports itself, and the two lines together say classification and
emission:
[segview] …::N$6 verdict=fires … code_point_at=1 regexp_test_dynamic=2 materialise=0
[segview-lower] __destruct_118613 open=1 next=1 segment=1 code_point_at=0 regexp_test=0
declined=none (classifier: code_point_at=1 regexp_test=2 materialise=0)
Note deliberately that the emission line reports `code_point_at=0
regexp_test=0` even on a site the classifier scores as fully answerable. That
is not a bug and it is not rounding: v1 emits `_segment` once per step and
leaves the body untouched, so no use is answered from the view yet. The gap
between the two lines IS the v1/v2 boundary, and having the instrument state it
is better than having a reader infer from the design that v1 already routes
`codePointAt` through the cursor. It will close when the per-use rewrite lands.
Also fixes an `unused_mut` this pass introduced in `max_local_id_in_module`,
which would have failed a `-D warnings` gate. Found by type-checking against
the existing release artifacts -- zero disk cost, which mattered because the
box is at 8 GiB and the integration build was killed by a disk watchdog.
Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp
(cherry picked from commit e4ce2a8)
…ising nothing v1 bound the segment with `_segment` once per step and left the body alone, so it removed the record and kept the substring. v2 rewrites the USES: on a site where the classifier found nothing that needs the string, the accepted path materialises nothing at all and the loop reaches zero allocations per grapheme. That is where the remaining time is. perry-b4's I2 table puts ink's wrapText subtree at 80.2 % of active main-thread CPU (`E46`/`tI1` 4,238 of 5,152 samples, `u_N_24_6` 4,131 — about 4.1 s of a 5.2 s turn), with no dominant collector leaf left; the collector's share is minors landing inside this loop. v1 does not reach that. v2 does. Two substitutions, with very different risk. `O.codePointAt(k)` becomes `js_segments_view_code_point_at(cursor, k)` — a pure expression swap. `k` is unchanged: it is segment-relative and segment-bounded by the runtime's contract (§9d), the same bound the materialised substring had. `recv.test(O)` is the hard one. Read from #9870 rather than assumed: `js_segments_view_regexp_test(cursor, regex)` — CURSOR FIRST — returns true, false, or `undefined` meaning "I declined" (global/sticky regex, patched `RegExp.prototype.test` or an own `test`), and the runtime does NOT fall back internally, so the compiler must. `recv` is arbitrary — in cc it is `g54.default()`, an opaque call that must run exactly once per evaluation — so it cannot be repeated in the fallback arm. The emitted form is a pure expression, so no control flow is restructured: Sequence([ LocalSet(t_recv, <recv>), // opaque call, ONCE LocalSet(t_res, _regexp_test(cursor, t_recv)), t_res === undefined ? t_recv.test(_segment(cursor)) : t_res ]) The materialisation is inside the decline arm, so the accepted path allocates nothing. Every rewritten use is GUARDED, not replaced: `cur != 0 ? <view form> : <original>`. The loop body is shared between the accepted and declined paths, so the original expression must survive for the decline arm, where `O` holds a real string. On acceptance `O` is bound to `undefined` and never read, because every use takes the view arm — which is what makes the accepted path allocation-free without duplicating the body. A site with even one unanswerable use stays on v1: paying per-use guards on top of a materialisation that happens anyway is strictly worse. WHAT SUBSTITUTES FOR THE TESTS THIS COULD NOT BE RUN AGAINST. This box cannot build (its target was deleted to recover disk), so `rustfmt` and reading are the only gates. The pass therefore rewrites a CLONE of the body and keeps it only if the emission matches the classification exactly — same `code_point_at` count, same `regexp_test` count. If they disagree, some use was not rewritten and would read an unbound segment on the accepted path, so the clone is discarded and v1 is used. The check is the mechanism, not a comment. `[segview-lower]` now reports which arm was taken, so classifier and emission can be compared on the real bundle: [segview-lower] <rec> open=1 next=1 segment=0 code_point_at=1 regexp_test=2 declined=none (v2: …) [segview-lower] <rec> open=1 next=1 segment=1 code_point_at=0 regexp_test=0 declined=none (v1: …) Decline paths are unchanged. Three HIR-level tests added beside the v1 ones. NOT COMPILED AND NOT RUN — see the commit message above and §v2 of HANDOFF_segview_e2e.md for exactly what is unverified. Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp (cherry picked from commit 089cb3b)
The lowering emitted the calls and the module never declared them, so the
in-process LLVM parse rejected the whole module:
perry_llvm_….ll:5172:22: error: use of undefined value '@js_segments_view_next'
%r75 = call double @js_segments_view_next(double %r74)
Not a degraded build — no build at all. The five entry points are now
registered in `runtime_decls/strings.rs` beside `js_for_of_next`, which is
where every other runtime native gets its `declare`.
Signatures are read from `perry-runtime/src/intl/segments_view.rs`, not
assumed: `open(f64,f64)`, `next(f64)`, `code_point_at(f64,f64)`,
`segment(f64)`, `regexp_test(f64,f64)`. Note `regexp_test` is (cursor, regex),
cursor first; it was relayed the other way round once and the source settled
it.
Why the tier's twelve HIR tests could not catch this: they assert the rewrite
emits `Call(ExternFuncRef "js_segments_view_next", …)`, and it did. The gap was
between "the lowering emits the call" and "the module can be parsed", and
nothing tested the second. `every_segment_view_entry_point_is_declared` closes
it by running the real declare phase over an `LlModule` and checking each of
the five by name — remove any one registration and it fails naming that symbol.
It also asserts ARITY, which is the sabotage a name-only check would miss: a
wrong parameter count parses cleanly and then miscompiles the call, because
LLVM will coerce or drop an argument rather than complain.
(cherry picked from commit ed356f7)
`v2_answers_every_use_from_the_view_and_materialises_nothing` failed because
the v2 rewriter handled only the generic `Call(PropertyGet(recv,"test"), [O])`
shape. perry folds a test whose regex is statically known into
`Expr::RegExpTest { regex, string }`, which the classifier counts as
`regexp_test_static` — so the classification said "answerable" and the emission
did not answer it.
The pass's own agreement check caught that: emission counts did not match
classification counts, so it discarded the rewrite and fell back to v1 rather
than emitting a loop that reads an unbound segment on the accepted path. The
guard did its job; this teaches the rewriter the shape so the guard stops
having to.
Unlike the generic form, the static node's regex is a literal or a binding with
no side effect worth hoisting, so it can be repeated in the decline arm and
needs one temporary rather than two.
Also corrects an assertion in that test that could not hold: it required
`__segview_test_recv` on a body whose only test is the static node, which has
no opaque receiver to hoist. That property belongs to the generic form and is
already pinned by `v2_evaluates_an_opaque_test_receiver_exactly_once`. Replaced
with the tri-state temporary, which this body does have, and commented so it is
not re-added.
16/16 segview tests pass.
(cherry picked from commit 6b522fb)
#9886's last-wins accessor record pushed lower_decl/class_decl.rs to 2050 lines. The member-shape helpers — computed-key naming, the accessor-name survey, and record_class_accessor itself — move to a sibling child module beside the existing class_heritage/member_registration. Unlike the page_meta split, this adds a child rather than renaming the parent, so nothing keyed on the path `lower_decl/class_decl.rs` moves; the two prose references to it elsewhere in the tree stay correct.
It is used only inside member_helpers itself, so importing it back into class_decl fails `-D warnings`. Visible to the gate but not to a plain `cargo check -p perry-hir`.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (28)
📝 WalkthroughWalkthroughThe pull request adds an optional ChangesSegment-view lowering
Node module compatibility
Class accessor semantics
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CompilerPipeline
participant SegViewCollector
participant SegViewRewriter
participant SegmentViewRuntime
CompilerPipeline->>SegViewCollector: collect segment for-of sites
SegViewCollector-->>CompilerPipeline: return verdicts and use tallies
CompilerPipeline->>SegViewRewriter: rewrite firing sites
SegViewRewriter->>SegmentViewRuntime: open and advance segment view
SegmentViewRuntime-->>SegViewRewriter: return cursor and segment results
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Sep 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge train: #9859, #9884, #9885, #9886, #9887.
#9859 is the compiler half of the
Intl.Segmenterview mode; its runtime half (#9870) landed in #9888, so the two are now together.Conflict resolutions
#9859 and the already-landed #9857 each add a build-cache exclusion for their own diagnostic env var (
PERRY_SEGVIEW_DIAG,PERRY_NATIVEINST_DIAG). Git read them as competing edits to one spot; they are independent, and both need the exclusion for the same reason — a cached build reuses the finished binary and never lowers HIR, so the counter prints nothing, and "nothing" reads exactly like "the tier never fired". Both blocks kept. A later commit in #9859 renumbers its own reference#9846→#9843, which re-conflicted against the merged form; the renumber is applied to the merged text rather than taking either side whole.File cap
#9886's last-wins accessor record pushed
lower_decl/class_decl.rsto 2050 lines. Its member-shape helpers — computed-key naming, the accessor-name survey, andrecord_class_accessoritself — move to a sibling child beside the existingclass_heritage/member_registration.This adds a child rather than renaming the parent, so
lower_decl/class_decl.rsstill exists and nothing keyed on that path moves. (Thearena/page_meta.rssplit in #9883 renamed its parent and cost four separate rounds of retargeting allowlists, gate JSON, and tests that read the file by path.)Audit note on #9886
ClassDecl::getters/settersare consumed withiter().find(...), so the FIRST entry with a name wins at lookup, while ECMA-262 says a later definition of the same key replaces the earlier one — Perry returned the shadowed accessor. The fix keys replacement on(name, is_static)rather than the name alone, which is right: a static and an instance accessor may legally share a name because they are distinct properties on different objects.Validation
run_lint_gates: all 64 gates passed; 2 CI-only skippedThe suites ran on the tree one commit earlier; the only change since is deleting an unused import that
-D warningsrejected, which cannot alter behaviour. The lint tier was re-run in full on the final tree.Summary by CodeRabbit
Performance
Intl.Segmenterfor…ofloops, reducing unnecessary segment-string materialization.Bug Fixes
node:perf_hooks,node:trace_events,fs/promises, andstream/promises.Diagnostics